Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for package exports into .pnp.js #1359

Closed
wants to merge 1 commit into from

Conversation

bgotink
Copy link
Sponsor Member

@bgotink bgotink commented May 14, 2020

This is just a first iteration, and even then the code is far from finished.

  • This needs a lot more tests & documentation
  • I'd love feedback on the pnp hook code, as this is the first time I'm touching PnP resolution

What's the problem this PR addresses?

Berry doesn't support exports in package.json

#650, #1277

How did you fix it?

This PR currently contains a very basic approach which loads the exports at resolution time, validates the exports and then applies the exports onto the unqualified path to get a new unqualified path. The algorithm has been implemented as per node documentation (https://nodejs.org/api/modules.html#modules_all_together, https://nodejs.org/api/esm.html#esm_resolver_algorithm).

As I noted in #650 (comment), package exports resolution currently fits neither in resolveToUnqualified or resolveUnqualified, so I've placed it into a separate function.
Alternatively we could validate the exports at installation time and store it in PnP data. That will result in "invalid metadata" errors at installation time instead of at resolution time (fail fast). It also means one less file system hit per bare import resolution. Finally, it'll make it possible for resolveToUnqualified to handle export mapping.

I'm not 100% happy with the current approach, if only because resolveToUnqualified returns an invalid path for packages that have exports. It feels weird to then go map that onto a valid path.

@arcanis
Copy link
Member

arcanis commented May 15, 2020

Alternatively we could validate the exports at installation time and store it in PnP data. That will result in "invalid metadata" errors at installation time instead of at resolution time (fail fast). It also means one less file system hit per bare import resolution. Finally, it'll make it possible for resolveToUnqualified to handle export mapping.

I think we should refrain from doing that - third-party resolvers that already make use of resolveToUndefined (for example the Webpack plugin) won't be aware that the exports field already got resolved and that could lead to weird behaviors.

Overall I'm a bit worried of the size of the implementation - it's a lot of code just to workaround the fact that we cannot directly call the equivalent of resolveUnqualified built within Node. I wonder if we could instead just contribute a function upstream that we could use if available?

I've also opened nodejs/node#33423 which, if fixed, would simplify that a bit (we would still need a way to access the resolver at mid-level, since otherwise we would send absolute paths to the resolver which would then ignore the exports field).

@bgotink
Copy link
Sponsor Member Author

bgotink commented May 15, 2020

Overall I'm a bit worried of the size of the implementation - it's a lot of code just to workaround the fact that we cannot directly call the equivalent of resolveUnqualified built within Node. I wonder if we could instead just contribute a function upstream that we could use if available?

It's not just the 200+ lines of code that worry me. The fact that we implement the algorithm ourselves is potentially an issue. Right now it would work fine, but it breaks down if changes to this algorithm are made in the future. For example, something they've left open in the algorithm: re-exporting packages. Here's an example of how a package could use that to conditionally load a CSS preprocessor:

{
  "peerDependencies": {
    "sass": "*",
    "node-sass": "*"
  },
  "peerDependenciesMeta": {
    "sass": {"optional": true},
    "node-sass": {"optional": true}
  },
  "exports": {
    "./sass": ["sass", "node-sass", "./no-sass.js"]
  }
}

We would need to change the behaviour in .pnp.js. In other words, to get newer node functionality I would have to update yarn. A disconnect between the version of the export algorithm used in yarn and the version used in node will lead to confusion.

Moving this to a node API would benefit not just us but—I hope—the entire ecosystem of bundlers and loaders. Right now node has documented the algorithm but hardcoded it into the cjs and esm loaders. If e.g. typescript wants to support typings exports for .d.ts files, they have to copy the algorithm entirely to be able to pass in options other than the hardcoded ['node', 'require'] and ['node', 'import'].

Without support in node itself, typescript is stuck between a rock and a hard place:

  • require every package to explicitly export the package.json file; or
  • implement the node module resolution algorithm in their own code

I'm using typescript as example here, but the same goes for any project that reads a file path out of the package manifest. Browserify, webpack, rollup etc. might want to pass in extra / different options, e.g. 'browser'. Typescript and the Angular CLI add non-js targets in the exports.

The Angular CLI has just moved from a custom implementation of the node_modules algorithm to using require.resolve in @angular/cli 8.3. It would be a shame if they had to move back because package exports happened.

// node_modules/with-exports/package.json
{
  "name": "with-exports",
  "exports": "./index.js"
}
// test.js
console.log(require.resolve('with-exports/package.json'));
bram:/tmp/test $ nvm run 14 test
Running node v14.0.0 (npm v6.14.4)
internal/modules/cjs/loader.js:492
  throw new ERR_PACKAGE_PATH_NOT_EXPORTED(basePath, mappingKey);
  ^

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './package.json' is not defined by "exports" in /private/tmp/test/node_modules/with-exports/package.json
    /* boring stacktrace */ {
  code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
}


bram:/tmp/test $ nvm run lts/dubnium test
Running node v10.20.1 (npm v6.14.4)
/private/tmp/test/node_modules/with-exports/package.json

@arcanis
Copy link
Member

arcanis commented May 15, 2020

Moving this to a node API would benefit not just us but—I hope—the entire ecosystem of bundlers and loaders. Right now node has documented the algorithm but hardcoded it into the cjs and esm loaders.

Yep, I totally agree with that. Imo the best would even be for the Node project to publish an official resolve-like package, but even simply exposing the pipeline primitive algorithm in a generic way would be very welcome. It's almost like every external project is expected to reimplement it, plus bugs.

@guybedford I'm curious what you would think about that, since you worked quite a bit on the Node resolution pipeline if I remember correctly? Do you think there could be any chance for something like this to happen?

@guybedford
Copy link

@arcanis I agree the ideal would be an official package. If someone is interested in working on this I'm sure it could get momentum - the main thing is that a userland package needs hooks into resolver features like extensions etc, so working out those details needs to be done.

Personally my hope was that by putting together a very clear specification for the resolver, it would be easy for other tools to build on top of it since the semantics are well-defined. For example in jspm I borrow semantics as necessary but don't need an actual resolver implementation since the package lookup process is so different.

I am surprised no one has put together a very good userland resolver implementation yet and would encourage people to work on either this problem or trying to get that work directly in core too. I do think userland may be a better fit to ensure speed of changes that meet all the configurations and userland needs, but if you feel core would be better I have nothing against that too.

@guybedford
Copy link

Note the specification here describes the edge cases and errors in detail, and has been carefully maintained to match the implementation - https://nodejs.org/dist/latest-v14.x/docs/api/esm.html#esm_resolver_algorithm (you have to expand the "Resolver algorithm specification" section).

@arcanis
Copy link
Member

arcanis commented Jun 1, 2020

We've discussed with @bgotink and came to the conclusion that in the current state of things, maybe the best would be to contribute the exports support to the browserify/resolve package, since it's the closest implementation of a "standard" resolver there is at the moment (cc @ljharb).

This way we could remove the qualified path resolution from our codebase, focusing exclusively on bare specifiers.

@arcanis arcanis closed this Jun 1, 2020
@guybedford
Copy link

This sounds like a great collaboration, thank you @arcanis for taking the time to dig deeper here and find a solution that can help many other projects too!

If there are any questions or issues integrating the resolution changes, I'm sure @ljharb will be able to advise on this, and feel free to ping me as well. And if there are any difficulties or feedback you have on the implementation or spec do feel free to post an issue on the modules repo too.

@ljharb
Copy link

ljharb commented Jun 1, 2020

Happy to help any way I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants